Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add githubReleaseAssets option #809

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

esatterwhite
Copy link

Includes the addition of a new option githubReleaseAssets to replace the old assets option. The assets option is identical to the option found in the git plugin but is ambiguous when used in shared configuration packages.

Fixes: #808

@esatterwhite esatterwhite force-pushed the esatterwhite/disambiguate-assets branch from 1aed3e4 to 0370eb5 Compare April 10, 2024 14:37
Includes the addition of a new option githubReleaseAssets to replace the
old assets option. The assets option is identical to the option found in
the git plugin but is ambiguous when used in shared configuration
packages.

Fixes: semantic-release#808
@esatterwhite esatterwhite force-pushed the esatterwhite/disambiguate-assets branch from 0370eb5 to 8a17388 Compare April 17, 2024 16:30
@babblebey
Copy link
Member

Having seen your issue #808, I do not subscribe to introducing a new config property githubReleaseAssets as the fix for the issue for the following reasons..

  • replacing assets with githubReleaseAssets will be BREAKING CHANGE (avoiding that as much as possible)
  • it removes the simplicity from the naming and makes our available plugin apis less uniform
  • the eco-system impact: imagine needing to make similar changes with the gitlab/bitbucket and every hosted git platform plugins too

Thinking real loud here, I believe we can do better by leaning towards a priority styles solution where the level of definition of the assets property determines how its set 🤔

@esatterwhite
Copy link
Author

  • replacing assets with githubReleaseAssets will be BREAKING CHANGE (avoiding that as much as possible)

This still uses assets if githubReleaseAssets is not set. So it doesn't break anything.

  • it removes the simplicity from the naming and makes our available plugin apis less uniform

ok, so we rename it releaseAssets (or something similarly generic) rather than specifically githubReleaseAssets. That is as simple as githubApiPathPrefix. Doing so will allow the config options to be the same across different plugins.

To be fair, the simplicity in naming here is the thing that makes it difficult to use. Which is the more important thing

  • the eco-system impact: imagine needing to make similar changes with the gitlab/bitbucket and every hosted git platform plugins too

I think the above change addresses this.

Thinking real loud here, I believe we can do better by leaning towards a priority styles solution where the level of definition of the assets property determines how its set 🤔

That would also a breaking change. a default will have to be put in place, and it more than likely will not match what people have in place. I additionally postulate that breaks the simplicity ideal. It also means that other plugins will have to follow suite. It is not a common pattern in the semantic-release eco-system.

@babblebey
Copy link
Member

That would also a breaking change. a default will have to be put in place....

This is the way the config should just operate... when there's an assets property defined within the plugin property or extends then it should be prioritized over any other assets definition in the scope above it.

I can easily assume that this is how the package currently functions, but considering that it isn't (I still want verify this behavior), we gotta just fix exactly that 🤔

Let's continue this discussion in #808

@esatterwhite
Copy link
Author

esatterwhite commented Aug 13, 2024

This is the way the config should just operate... when there's an assets property defined within the plugin property or extends then it should be prioritized over any other assets definition in the scope above it

The problem isn't priority of resolution. the problem is that a single key in a json object cannot represent 2 different settings

This is the way the config should just operate...

I don't disagree, My point here is that the argument against a breaking change isn't really relevant, as it would potentially be a breaking change in either case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

assets option is ambiguous with git plugin assets option
2 participants